-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Auto detect background image #7849
Conversation
…utoDetectBackgroundImage
This comment has been minimized.
This comment has been minimized.
I'm wondering if this would be better suited as a parameter for the "backgroundImage" setting we already have. Such as:
|
@cinnamon-msft I initially tried to implement that, but after talking with Christ-Jan I thought it might be easier on the user to have a setting dedicated for this. A true/false value seems harder to mess up. I could add to the documentation to clarify that if true, it will ignore the "backgroundImage" setting. Looking into the future with the settings GUI, it would be easier to convert the dedicated setting into the GUI. We could have it as a checkbox which would then remove the option to fill out any other background options if checked. Or some idea along those lines. With that being said, your suggestion would be less code, which would be easier to maintain. I am open to reworking the issue if the team thinks having a keyword would be more valuable than a dedicated setting. |
I disagree with that, actually. Having multiple settings control which background image to use adds more confusion. It makes more sense to just have
That's true, but the Settings UI doesn't have to align 1-to-1 with the json. We can still have your proposed GUI implementation, and just edit the JSON accordingly (which is probably what we'll do).
Kayla has a good handle on these user-facing issues so I agree with her. I recommend you keep an eye on |
@carlos-zamora thanks for the feedback. I have made the changes. I am a bit sad how much easier @cinnamon-msft's implementation was. Would you rather have me update this PR or close and reopen with the new code? |
100% update this PR haha. That way we can track the whole thing more easily. Feel free to reach out to @cinnamon-msft or I if you need any help with anything :). And thanks for the contribution! (should've started with that haha) |
@carlos-zamora and @cinnamon-msft both the code and documentation has been updated. Please review both when you have the time. 😄 Documentation: MicrosoftDocs/terminal#155 |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not so sure that this is the proper design. You're overwriting the BackgroundImagePath
during a validation phase. I think at any time, the BackgroundImagePath
shouldn't be changed, actually. Instead, it should be interpreted differently. See my thoughts on how expanding the path works below.
How Expanding the Background Image Path Works
1. We read the JSON (Profile.cpp) 2. We directly copy the text to the Profile object 3. When a TermControl is created, we specifically request the `ExpandedBackgroundImagePath` and resolve the file path then. 4. TermControl now has the full path and retrieves the background imageStep 3 is the important one. We don't modify Profile
.
Consider the Settings UI. We want to present the value the user gave us, not the one that it resolves to. We specifically only want to resolve the value when it's needed.
So here's my suggestions:
- "DesktopWallpaper" should be a static string off of Profile
- Move this logic to
ExpandedBackgroundImagePath
(that way it's expanded when it's consumed) - Update
_ValidateMediaResources
saying that a value of DesktopWallpaper (reference the static string in bullet 1) is acceptable - Add a quick test showing that if you set the background image to "DesktopWallpaper" in the json, it gets exposed as "DesktopWallpaper" in
profile.BackgroundImage()
but something else inprofile.ExpandedBackgroundImage
.
… normal path. In addition, added a deserialization test
New misspellings found, please review:
To accept these changes, run the following commands
✏️ Contributor please read thisBy default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.
If the listed items are:
See the 🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The :check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You didn't have to update _ValidateMediaResources
? I expected that when you set the background image to DesktopWallpaper
, Terminal would give you a popup warning that it's not valid.
And don't worry about "slowing down", happy to help anyway I can. And hey, the more you learn, the more you can jump into the code! :) Thanks for the help!
@carlos-zamora I believe I have the spell check figured out, but waiting to confirm that. All of your comments should be resolved now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove that. That's probably why the CI build failed? I'd double check by build/deploy-ing Terminal after you remove that.
After that change is in, I'll approve :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! I forgot to mention how to format your code! Run this with PowerShell and it should do it all for you:
Import-Module .\tools\OpenConsole.psm1
Invoke-CodeFormat
Approving since that's a quick and easy fix. CI seems to be failing for some unrelated reason aside from that.
As for other issues you can work on, take a look at our Easy-Starter
issues here. You're more than welcome to tackle some other issues on our repo too. If you start one though, I suggest you post on the issue saying that you're gonna try and do it (and if you have any related questions). That way the team has an idea of what's going on 😊 Thanks!
@carlos-zamora Thanks for all the help. @cinnamon-msft would you mind being a second reviewer? I pushed the formatted code. |
I'd prefer if someone else from @microsoft/windows-console-team reviewed this since they are more familiar with the codebase. |
@cinnamon-msft works for me. I don't know any other devs, so you might have to tag one 😅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If @cinnamon-msft is cool with this value being DesktopWallpaper
, then I'm fine with this
Don't worry about the CI failure - that's resolved over in #7930 |
@zadjii-msft @cinnamon-msft @carlos-zamora @PankajBhojwani |
Hello @zadjii-msft! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
🎉 Handy links: |
Summary of the Pull Request
Added watch on desktopImagePath to check when the path equals "DesktopWallpaper"
If it does equal "DesktopWallpaper" it replaces the path with a path to the desktop's wallpaper
*I am a student and this is my first pull request for Terminal so please give feedback no matter how small. It's the best way I can learn.
PR Checklist
Detailed Description of the Pull Request / Additional comments
I am using SystemParametersInfo for SPI_GETDESKWALLPAPER which puts the path into a WCHAR and that is then inserted as the BackgroundImagePath.
I do not think an additional test would add value. The SPI_GETDESKTOPWALLPAPER uses the computers local wallpaper path and puts it into a WCHAR, which then I feed into BackgroundImagePath() as it's new path. I don't think there adds value in making a static path of the desktop background and testing that, given that static tests are already done for "BackgroundImage()".
Validation Steps Performed
(Manual Validation - Test False Value)
(Manual Validation - Test True Value)
(Manual Validation - Multiple Tabs True Value)